gh-143231: Do not swallow not matched warnings in assertWarns*()#149229
Conversation
unittest.TestCase methods assertWarns() and assertWarnsRegex() no longer swallow warnings that do not match the specified category or regex. Nested context managers are now supported.
Documentation build overview
41 files changed ·
|
bitdancer
left a comment
bitdancer
left a comment
There was a problem hiding this comment.
Has the behavior of record=True changed at all?
| self.assertWarnsRegex(UserWarning, 'foo', self.assertMessagesCM, | ||
| 'assertWarnsRegex', (UserWarning, 'regex'), |
There was a problem hiding this comment.
When I read this I thought at first that you were calling assertWarnsRegex directly instead of calling assertMessagesCM. Even after understanding what you are actually doing it is hard to understand exactly what this is testing. If I understand the difference correctly (now it emits the warning where before it didn't, but that's not what the test is testing), then I think it would be easier to understand if you wrote it like this:
| self.assertWarnsRegex(UserWarning, 'foo', self.assertMessagesCM, | |
| 'assertWarnsRegex', (UserWarning, 'regex'), | |
| with self.assertWarnsRegex(UserWarning, 'foo'): | |
| self.assertMessagesCM('assertWarnsRegex', (UserWarning, 'regex'), |
There was a problem hiding this comment.
This test is for the callable interface of assertWarnsRegex(). The next test is for the context manager interface. I think it is better to not mix them in these tests.
There was a problem hiding this comment.
Right, I understand that. But that part of the test is in the inner call to assertMessagesCM, which is the thing calling assertWarnsRexex multiple times using the callable interface. The fact that a warning that is not caught is also emitted is a consequence of the nature of the test, but that's a byproduct, and by using the assertWarns context manager we indicate that we are expecting that byproduct.
If you want to leave it the way you have it I won't block on it ;)
There was a problem hiding this comment.
Oh, I missed this with other tests in test_case where I use nested callables. Here, I used the callable form only because it was the minimal change. But a context manager looks more readable.
| exc_name = str(self.expected) | ||
| first_matching = None | ||
| matched = False | ||
| not_matching_warnings = [] |
There was a problem hiding this comment.
Standard English would make this variable name non_matching_warnings.
|
When you're done making the requested changes, leave the comment: |
…ing by module name. Add the module attribute to warnings.WarningMessage.
|
I fixed most of the remaining issues with minimal changes in the It required more complex changes in |
|
Changes to the |
2 participants
unittest.TestCase methods assertWarns() and assertWarnsRegex() no longer swallow warnings that do not match the specified category or regex. Nested context managers are now supported.